-
Notifications
You must be signed in to change notification settings - Fork 20
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add sct and cert verification #1
Conversation
b78b894
to
8bc998d
Compare
@di here is working sct/cert validation. There are sample in src/test/resources. They are generated off of my email, so it does feel weird to include them in a python client, but you could get away using them to test your code for now. |
8ffeba2
to
98fe347
Compare
src/main/java/dev/sigstore/fulcio/client/SigningCertificate.java
Outdated
Show resolved
Hide resolved
98fe347
to
d3f3635
Compare
5b21366
to
1d72526
Compare
de94559
to
f37a67b
Compare
f37a67b
to
2f1f53e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lots of mysterious stuff in here, but looks like you know what you're doing!
My meta comment is that this would be easier to review broken up into neater PRs. For instance the refactorings could have been separated out. No biggie though.
public CertificateResponse(CertPath certPath, @Nullable byte[] sct) { | ||
this.certPath = certPath; | ||
this.sct = sct; | ||
public class FulcioValidationException extends Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's kind of weird to fork a response into an exception
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this extend RuntimeException?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, we want the tool to catch this and present it to the user. I don't think it's a catastrophic failure. It could be a misconfigured private key, or the wrong fulcio or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. Makes sense. We should chat about our approach here some time with Jason. I'll typically avoid throwing any checked exceptions unless it's type or api enables the client in some way.
|
||
public static FulcioValidator newFulcioValidator( | ||
byte[] fulcioRoot, @Nullable byte[] ctfePublicKey) | ||
throws InvalidKeySpecException, NoSuchAlgorithmException, CertificateException, IOException, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious why we allow the ctfePublicKjey to be null if that'll just result in a validation exception when we call validateSct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it can be null because scts aren't required to be returned from fulcio. they should be, but people could be running private instances and maybe don't care about internal certificate transparency (but maybe they should?)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be enforced via a policy/config?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But validate will always fail as a result won't it?
src/main/java/dev/sigstore/fulcio/client/SigningCertificate.java
Outdated
Show resolved
Hide resolved
e7b5663
to
16db253
Compare
16db253
to
5e71715
Compare
- Rename CertificateResponse to SigningCertificate - SCTs are optional - Add more testing around parsing - Add example fulcio/ctfe public keys and sct/cert examples for testing - Exceptions are still kinda just passed along Signed-off-by: Appu Goundan <[email protected]> Co-authored-by: Vladimir Sitnikov <[email protected]>
5e71715
to
adc9c2a
Compare
Add sct and cert validations